Mariano/stuff#74
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update adds comprehensive evidence management features across the application. New database models, migrations, and seed routines have been introduced to support evidence and organization evidence. On the frontend, multiple server actions, UI components, hooks, and pages have been added for file uploading, deletion, URL management, and data retrieval. Layout adjustments improve centering across various pages, and new localization entries and menu items support the evidence feature. Additional dependency updates further extend AWS S3 functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as FileUploadComponent
participant H as useFileUpload Hook
participant A as AuthActionClient (getUploadUrl)
participant S as AWS S3
U->>F: Selects and drops a file
F->>H: Pass file to upload handler
H->>A: Request presigned upload URL
A->>S: Generate signed URL for PUT
S-->>A: Return signed URL
A-->>H: Return upload URL and file URL
H->>S: Upload file via PUT request
S-->>H: Acknowledge upload
H-->>F: Trigger onSuccess callback
sequenceDiagram
participant U as User
participant D as EvidenceDetails Component
participant P as useFilePreview Hook
participant A as AuthActionClient (getEvidenceFileUrl)
participant S as AWS S3
U->>D: Requests file preview
D->>P: Invoke getPreviewUrl with file URL
P->>A: Request signed URL for preview
A->>S: Validate and generate preview URL
S-->>A: Return preview URL
A-->>P: Provide preview URL
P-->>D: Display file preview to user
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (54)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/uploadEvidenceFile.ts (3)
9-11: Validate environment variables at build time
These checks are performed at runtime. It is recommended to consolidate them into a configuration loader or a boot-up validation routine, ensuring a fail-fast mechanism during startup rather than runtime to minimize unexpected behavior.
36-46: Consistent approach to error handling
While the main logic returns{ success: false, error }for business rule validation, the AWS credential check at lines 9-11 throws an error directly. Adopting one coherent pattern (either throwing exceptions or returning structured responses) throughout the file streamlines error handling and makes behavior more predictable.
80-88: Consider concurrency conflicts with array fields
Appending file URLs to an array in concurrent uploads can result in a last-write-wins situation, potentially overwriting other updates. A separate table or more robust transactional approach may mitigate race conditions and data-loss risks.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/getEvidenceFileUrl.ts (3)
9-19: Centralize environment checks
These environment variable checks repeat across multiple files. A shared configuration loader would help to keep the code DRY and ensure a single source of truth for AWS credentials and settings.
34-47: Guard against unusual or region-specific S3 URLs
While this regex-based extraction handles standard patterns, consider using robust URL parsing utilities to handle unexpected edge cases or region-specific S3 domains. This can help avoid future maintenance headaches.
66-80: Unify error handling patterns
Here, errors are thrown instead of returning a consistent structure (e.g.,{ success: false, error }). Aligning this approach with other action files enables uniform handling of errors downstream and simplifies client-side response parsing.apps/app/src/types/actions.ts (1)
1-5: Adopt the new interface in other actions
ThisActionResponseinterface promotes a uniform return type. Consider adopting it in recently added actions likegetEvidenceFileUrlto avoid mixing thrown errors with structured responses and provide a consistent experience throughout the application.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table-header.tsx (1)
13-13: Add type safety for column headers.The type cast to string could lead to runtime errors if the header is not a string. Consider adding type checking or using a type guard.
Apply this diff to improve type safety:
- {column.columnDef.header as string} + {typeof column.columnDef.header === 'string' ? column.columnDef.header : String(column.columnDef.header)}apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/types.ts (2)
3-8: Consider adding URL validation.The URLs are typed as strings without validation. Consider using a URL validation utility or a custom type guard to ensure valid URLs.
Example implementation:
function isValidUrl(url: string): boolean { try { new URL(url); return true; } catch { return false; } } export interface UploadUrlResponse { uploadUrl: string & { __brand: 'ValidatedUrl' }; fileUrl: string & { __brand: 'ValidatedUrl' }; } // Usage: function validateUrls(response: { uploadUrl: string, fileUrl: string }): UploadUrlResponse { if (!isValidUrl(response.uploadUrl) || !isValidUrl(response.fileUrl)) { throw new Error('Invalid URL'); } return response as UploadUrlResponse; }
10-13: Add URL validation and array constraints.Consider adding URL validation and constraints for the
additionalUrlsarray to handle edge cases.Example implementation:
export interface UpdateUrlsResponse extends ActionResponse<{ additionalUrls: NonEmptyArray<ValidatedUrl>; }> {} type ValidatedUrl = string & { __brand: 'ValidatedUrl' }; type NonEmptyArray<T> = [T, ...T[]];apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileIcon.tsx (1)
9-19: Improve robustness and maintainability of file type detection.Consider the following improvements:
- Handle edge cases in extension extraction
- Move file type mappings to a configuration object
- Add support for more file types
Apply this diff to improve the implementation:
+const FILE_TYPE_CONFIG = { + image: ['jpg', 'jpeg', 'png', 'gif', 'webp', 'svg', 'bmp'], + document: ['pdf', 'doc', 'docx', 'txt', 'rtf', 'odt', 'pages'], +} as const; + export function FileIcon({ fileName }: FileIconProps) { - const extension = fileName.split(".").pop()?.toLowerCase(); + const extension = fileName.includes('.') ? fileName.split('.').pop()?.toLowerCase() : ''; - if (extension && ["jpg", "jpeg", "png", "gif", "webp"].includes(extension)) { + if (extension && FILE_TYPE_CONFIG.image.includes(extension)) { return <FileImage className="h-12 w-12 text-muted-foreground" />; } - if (extension && ["pdf", "doc", "docx", "txt"].includes(extension)) { + if (extension && FILE_TYPE_CONFIG.document.includes(extension)) { return <FileText className="h-12 w-12 text-muted-foreground" />; } return <File className="h-12 w-12 text-muted-foreground" />; }apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useOrganizationEvidence.ts (2)
13-19: Improve error handling with specific error types.The current error handling doesn't distinguish between different types of errors, which could make debugging more difficult.
Apply this diff to improve error handling:
+class OrganizationEvidenceError extends Error { + constructor(message: string, public code: string) { + super(message); + this.name = 'OrganizationEvidenceError'; + } +} + if (!result || "error" in result) { throw new Error( typeof result?.error === "string" ? result.error : "Failed to fetch organization evidence" ); } + if (!result) { + throw new OrganizationEvidenceError('No result returned', 'NO_RESULT'); + } + if ('error' in result) { + throw new OrganizationEvidenceError( + typeof result.error === 'string' ? result.error : 'Unknown error', + 'API_ERROR' + ); + }
24-33: Enhance SWR configuration for better error handling and UX.Consider adding error retry configuration and loading state handling to improve the user experience.
Apply this diff to improve the implementation:
export function useOrganizationEvidence({ id }: UseOrganizationEvidenceProps) { return useSWR( ["organization-evidence", id], () => fetchOrganizationEvidence({ id }), { revalidateOnFocus: false, revalidateOnReconnect: false, + errorRetryCount: 3, + errorRetryInterval: 1000, + loadingTimeout: 3000, + onLoadingSlow: () => { + console.warn('Slow loading detected for organization evidence'); + }, + onError: (error) => { + console.error('Failed to fetch organization evidence:', error); + }, } ); }apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/data-table/data-table-header.tsx (3)
18-47: Improve component reusability.The component has hardcoded column names and styles which limits its reusability. Consider accepting column configurations as props.
Apply this diff to improve reusability:
type Props = { table?: { getIsAllPageRowsSelected: () => boolean; getIsSomePageRowsSelected: () => boolean; getAllLeafColumns: () => { id: string; getIsVisible: () => boolean; }[]; toggleAllPageRowsSelected: (value: boolean) => void; }; loading?: boolean; + columns: { + id: string; + label: string; + className?: string; + }[]; }; export function DataTableHeader({ table, loading, columns }: Props) { const isVisible = (id: string) => loading || table ?.getAllLeafColumns() .find((col) => col.id === id) ?.getIsVisible(); return ( <TableHeader> <TableRow className="hover:bg-transparent"> - {isVisible("type") && ( - <TableHead className="h-11 px-4 text-left align-middle font-medium"> - Type - </TableHead> - )} - {isVisible("description") && ( - <TableHead className="h-11 px-4 text-left align-middle font-medium"> - Description - </TableHead> - )} - {isVisible("status") && ( - <TableHead className="h-11 px-4 text-left align-middle font-medium"> - Status - </TableHead> - )} + {columns.map((column) => + isVisible(column.id) && ( + <TableHead + key={column.id} + className={`h-11 px-4 text-left align-middle font-medium ${column.className ?? ''}`} + > + {column.label} + </TableHead> + ) + )} </TableRow> </TableHeader> ); }
5-16: Refine Props interface to remove unused methods.The Props interface includes methods that aren't used in the component (
getIsAllPageRowsSelected,getIsSomePageRowsSelected,toggleAllPageRowsSelected). Consider refining it to only include what's needed.Apply this diff to simplify the Props interface:
type Props = { table?: { - getIsAllPageRowsSelected: () => boolean; - getIsSomePageRowsSelected: () => boolean; getAllLeafColumns: () => { id: string; getIsVisible: () => boolean; }[]; - toggleAllPageRowsSelected: (value: boolean) => void; }; loading?: boolean; };
18-47: Consider making the component more flexible and localizable.The component has hardcoded column labels and a fixed set of columns. Consider:
- Using a translation function for column labels
- Making columns configurable through props
Apply this diff to improve flexibility and localization:
+import { useTranslations } from "next-intl"; + +interface Column { + id: string; + label: string; +} + +const DEFAULT_COLUMNS: Column[] = [ + { id: "type", label: "evidence.type" }, + { id: "description", label: "evidence.description" }, + { id: "status", label: "evidence.status" } +]; + export function DataTableHeader({ table, loading }: Props) { + const t = useTranslations(); + const columns = DEFAULT_COLUMNS; + const isVisible = (id: string) => loading || table ?.getAllLeafColumns() .find((col) => col.id === id) ?.getIsVisible(); return ( <TableHeader> <TableRow className="hover:bg-transparent"> - {isVisible("type") && ( - <TableHead className="h-11 px-4 text-left align-middle font-medium"> - Type - </TableHead> - )} - {isVisible("description") && ( - <TableHead className="h-11 px-4 text-left align-middle font-medium"> - Description - </TableHead> - )} - {isVisible("status") && ( - <TableHead className="h-11 px-4 text-left align-middle font-medium"> - Status - </TableHead> - )} + {columns.map((column) => + isVisible(column.id) && ( + <TableHead + key={column.id} + className="h-11 px-4 text-left align-middle font-medium" + > + {t(column.label)} + </TableHead> + ) + )} </TableRow> </TableHeader> ); }apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileDelete.ts (3)
12-52: Add loading state to prevent duplicate requests.The hook should track loading state to prevent multiple delete requests while one is in progress.
Apply this diff to add loading state:
export function useFileDelete({ evidenceId, onSuccess }: UseFileDeleteProps) { const { toast } = useToast(); + const [isDeleting, setIsDeleting] = useState(false); const handleDelete = useCallback( async (fileUrl: string) => { + if (isDeleting) return; try { + setIsDeleting(true); const response = await deleteEvidenceFile({ evidenceId, fileUrl, }); if (!response?.data) { throw new Error("Failed to delete file"); } if (!response.data.success) { throw new Error(response.data.error || "Failed to delete file"); } await onSuccess(); toast({ title: "Success", description: "File deleted successfully", }); } catch (error) { console.error("Error deleting file:", error); toast({ title: "Error", description: error instanceof Error ? error.message : "Failed to delete file", variant: "destructive", }); + } finally { + setIsDeleting(false); } }, - [evidenceId, onSuccess, toast] + [evidenceId, onSuccess, toast, isDeleting] ); return { handleDelete, + isDeleting, }; }
7-10: Consider making onSuccess callback optional.The onSuccess callback might not always be needed. Consider making it optional to improve flexibility.
Apply this diff:
interface UseFileDeleteProps { evidenceId: string; - onSuccess: () => Promise<void>; + onSuccess?: () => Promise<void>; }
49-51: Add loading state for better UX.Consider adding a loading state to provide feedback during the deletion process.
Apply this diff:
+ const [isDeleting, setIsDeleting] = useState(false); + const handleDelete = useCallback( async (fileUrl: string) => { + setIsDeleting(true); try { // ... existing code ... } catch (error) { // ... existing code ... + } finally { + setIsDeleting(false); } }, - [evidenceId, onSuccess, toast] + [evidenceId, onSuccess, toast, setIsDeleting] ); return { handleDelete, + isDeleting, };apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidence.ts (3)
25-29: Add type safety with const assertions for error responses.Consider adding
as constto the error response object to improve type inference and prevent accidental modifications.- return { - success: false, - error: "Not authorized - no organization found", - }; + return { + success: false, + error: "Not authorized - no organization found", + } as const;
42-47: Add type safety with const assertions for not found response.Similarly, add
as constto the not found response object.- return { - success: false, - error: "Evidence not found", - }; + return { + success: false, + error: "Evidence not found", + } as const;
49-53: Add type safety with const assertions for success response.Complete the type safety improvements by adding
as constto the success response.- return { - success: true, - data: evidence, - }; + return { + success: true, + data: evidence, + } as const;apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/data-table/columns.tsx (2)
38-54: Improve status column maintainability and accessibility.The status logic could be more maintainable and accessible:
- Extract status logic to a separate function
- Add aria-label for screen readers
cell: ({ row }) => { - const requirement = row.original; - const isCompleted = - requirement.type === "policy" - ? requirement.organizationPolicy?.status === "published" - : false; + const isCompleted = getRequirementStatus(row.original); return ( - <div className="flex items-center justify-center"> + <div + className="flex items-center justify-center" + aria-label={isCompleted ? "Completed" : "Not completed"} + > {isCompleted ? ( <CheckCircle2 size={16} className="text-green-500" /> ) : ( <XCircle size={16} className="text-red-500" /> )} </div> ); }, + const getRequirementStatus = (requirement: RequirementType): boolean => { + return requirement.type === "policy" && + requirement.organizationPolicy?.status === "published"; + };
30-32: Add title attribute for truncated text.When using truncation, add a title attribute to show the full text on hover.
cell: ({ row }) => ( - <div className="truncate">{row.original.description}</div> + <div className="truncate" title={row.original.description || ""}> + {row.original.description} + </div> ),apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx (2)
28-28: Use translated error message.The error message should be translated using the i18n function.
- if (error) return <div>Error loading evidence tasks</div>; + if (error) return <div>{t("evidence.list.error")}</div>;
36-41: Add accessibility attributes to search input.Enhance accessibility by adding aria-label and role attributes.
<Input placeholder={t("common.filters.search")} onChange={(e) => handleSearch(e.target.value)} defaultValue={search || ""} className="max-w-sm" + aria-label={t("common.filters.search")} + role="searchbox" />apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/updateEvidenceUrls.ts (1)
47-55: Use database transaction for atomic updates.Wrap the database operations in a transaction to ensure atomicity.
- const updatedEvidence = await db.organizationEvidence.update({ - where: { id: evidenceId }, - data: { - additionalUrls: urls, - }, - include: { - evidence: true, - }, - }); + const updatedEvidence = await db.$transaction(async (tx) => { + return tx.organizationEvidence.update({ + where: { id: evidenceId }, + data: { + additionalUrls: urls, + }, + include: { + evidence: true, + }, + }); + });apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getAllOrganizationControlRequirements.ts (2)
9-11: Implement search functionality.The schema includes a
searchparameter, but it's not utilized in the query. Consider implementing search functionality to filter the results.where: { organizationControl: { organizationId: user.organizationId, + OR: search ? [ + { control: { name: { contains: search, mode: 'insensitive' } } }, + { control: { description: { contains: search, mode: 'insensitive' } } } + ] : undefined }, },
31-50: Consider implementing pagination for large datasets.The current implementation fetches all records without pagination, which could impact performance with large datasets.
interface GetAllOrganizationControlRequirementsInput { search?: string | null; + page?: number; + limit?: number; } const schema = z.object({ search: z.string().optional().nullable(), + page: z.number().optional().default(1), + limit: z.number().optional().default(10), }); // In the query const skip = (page - 1) * limit; await db.organizationControlRequirement.findMany({ where: { ... }, + skip, + take: limit, include: { ... }, });apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFilePreview.ts (2)
35-35: Remove console.log statement.Remove debug logging from production code.
- console.log("Preview URL response:", response);
26-64: Consider adding cleanup on unmount.The loading state might be left in an incorrect state if the component unmounts during the request.
const getPreviewUrl = useCallback( async (fileUrl: string): Promise<string> => { + const abortController = new AbortController(); setIsLoading(true); try { const response = await getEvidenceFileUrl({ evidenceId, fileUrl, + signal: abortController.signal, }); if (!response?.data) { throw new Error("Failed to get signed URL"); } const result = response.data as ActionResponse; if (!result.signedUrl) { throw new Error("Invalid signed URL response"); } return result.signedUrl; } catch (error) { + if (error.name === 'AbortError') { + return; + } console.error("Error getting file URL:", error); const errorMessage = error instanceof Error ? error.message : "Failed to load file preview"; toast({ title: "Error", description: errorMessage, variant: "destructive", }); throw error; } finally { setIsLoading(false); } }, [evidenceId, toast] ); + useEffect(() => { + return () => { + abortController.abort(); + }; + }, []);apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table.tsx (2)
30-35: Add accessibility attributes to the clickable row.The table row is clickable but missing proper accessibility attributes.
<TableRow key={row.id} data-state={row.getIsSelected() && "selected"} className="hover:bg-muted/50 cursor-pointer" + role="link" + tabIndex={0} + aria-label={`View evidence task ${row.original.id}`} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + router.push(`/evidence/${row.original.id}`); + } + }} onClick={() => router.push(`/evidence/${row.original.id}`)} >
14-21: Add loading state handling.Consider adding a loading state to improve user experience while data is being fetched.
-export function DataTable({ data }: { data: EvidenceTaskRow[] }) { +export function DataTable({ + data, + isLoading +}: { + data: EvidenceTaskRow[]; + isLoading?: boolean; +}) { const router = useRouter(); const table = useReactTable({ data, columns, getCoreRowModel: getCoreRowModel(), }); + if (isLoading) { + return ( + <div className="w-full h-24 flex items-center justify-center"> + <div className="animate-spin">Loading...</div> + </div> + ); + }apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/deleteEvidenceFile.ts (2)
43-70: Use transaction for database operations.Consider using a transaction to ensure data consistency when updating the evidence record.
try { + return await db.$transaction(async (tx) => { // Check if evidence exists and belongs to organization - const evidence = await db.organizationEvidence.findFirst({ + const evidence = await tx.organizationEvidence.findFirst({ where: { id: evidenceId, organizationId: user.organizationId, fileUrls: { has: fileUrl, }, }, }); if (!evidence) { return { success: false, error: "Evidence or file not found", }; } // Remove the file URL from the evidence record - await db.organizationEvidence.update({ + await tx.organizationEvidence.update({ where: { id: evidenceId }, data: { fileUrls: { set: evidence.fileUrls.filter((url) => url !== fileUrl), }, }, }); return { success: true, }; + }); } catch (error) {
63-70: Consider implementing soft delete.Instead of permanently removing the file URL, consider implementing a soft delete mechanism to maintain a history of file operations.
await db.organizationEvidence.update({ where: { id: evidenceId }, data: { fileUrls: { set: evidence.fileUrls.filter((url) => url !== fileUrl), }, + deletedFileUrls: { + push: { + url: fileUrl, + deletedAt: new Date(), + deletedBy: user.id, + }, + }, }, });apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidenceTasks.ts (2)
31-65: Consider adding pagination to the database query.While the current implementation works well for searching evidence tasks, it might benefit from pagination to handle large datasets more efficiently.
const evidenceTasks = await db.organizationEvidence.findMany({ + take: 10, // Limit results per page + skip: (page - 1) * 10, // Skip based on page number where: { organizationId: user.organizationId, ...(search
71-77: Consider adding more detailed error information for debugging.While the current error handling is good, it could be enhanced to include more context for debugging purposes while keeping the user-facing message generic.
} catch (error) { - console.error("Error fetching evidence tasks:", error); + console.error("Error fetching evidence tasks:", { + error, + organizationId: user.organizationId, + searchQuery: search + }); return {apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/data-table/data-table.tsx (2)
27-34: Consider extracting navigation logic to a custom hook.The row click handler contains business logic that could be reused across components. Consider extracting it to a custom hook for better maintainability and reusability.
+const useRequirementNavigation = () => { + const router = useRouter(); + + return useCallback((requirement: RequirementType) => { + if (requirement.type === "policy" && requirement.organizationPolicy?.policy?.id) { + router.push(`/policies/${requirement.organizationPolicy.policy.id}`); + } + }, [router]); +}; export function DataTable({ data }: DataTableProps) { const router = useRouter(); - const onRowClick = (requirement: RequirementType) => { - if (requirement.type === "policy" && requirement.organizationPolicy?.policy?.id) { - router.push(`/policies/${requirement.organizationPolicy.policy.id}`); - } - }; + const onRowClick = useRequirementNavigation();
66-66: Consider localizing the "No requirements found" message.The hardcoded message should be moved to a localization file for better internationalization support.
- No requirements found. + {t("common.no_requirements_found")}apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileUpload.tsx (3)
18-29: Consider moving file type restrictions to a configuration file.The accepted file types could be moved to a separate configuration file for better maintainability and reuse across components.
30-30: Consider making the file size limit configurable via environment variables.The hardcoded 10MB limit could be made configurable through environment variables for different deployment environments.
- maxSize = 10 * 1024 * 1024, // 10MB + maxSize = process.env.NEXT_PUBLIC_MAX_UPLOAD_SIZE_MB + ? parseInt(process.env.NEXT_PUBLIC_MAX_UPLOAD_SIZE_MB) * 1024 * 1024 + : 10 * 1024 * 1024, // Default to 10MB
49-84: Enhance accessibility by adding ARIA attributes.The file upload component could benefit from additional ARIA attributes to improve accessibility.
<div {...getRootProps()} + role="button" + aria-label="File upload area" className={cn(apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/SingleControl.tsx (2)
25-25: Consider adding a loading state component.Instead of returning null, consider showing a loading skeleton or spinner for better user experience.
- if (!control || !controlProgress) return null; + if (!control || !controlProgress) { + return <LoadingSkeleton />; + }
29-34: Consider extracting status calculation logic to a utility function.The status calculation logic could be moved to a separate utility function for better maintainability and testing.
+const getProgressStatus = (progress: { completed: number }): StatusType => { + if (progress.completed > 0) return "in_progress"; + return progress.completed === 0 ? "not_started" : "completed"; +}; - const progressStatus = - controlProgress?.progress?.completed > 0 - ? "in_progress" - : controlProgress?.progress?.completed === 0 - ? "not_started" - : "completed"; + const progressStatus = getProgressStatus(controlProgress.progress);apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx (2)
80-84: Enhance accessibility of status indicators.The status indicators use color alone to convey information. While the icons help, we should ensure WCAG compliance for color contrast and add ARIA labels.
- <XCircle size={16} className="text-red-500 shrink-0" /> + <XCircle + size={16} + className="text-red-500 shrink-0" + aria-label="Draft status" + /> )} <span className={ - isPublished ? "text-sm text-green-600" : "text-sm text-red-600" + isPublished + ? "text-sm text-green-600 font-medium" + : "text-sm text-red-600 font-medium" }Also applies to: 85-91
19-28: Consider memoizing tooltip content for performance.The tooltip components are recreated on every render. Consider memoizing them using
useMemofor better performance, especially with large datasets.+import { useMemo } from "react"; cell: ({ row }) => { - return ( + return useMemo(() => ( <TooltipProvider> <Tooltip> <TooltipTrigger asChild> <div className="max-w-[200px] truncate">{row.original.name}</div> </TooltipTrigger> <TooltipContent>{row.original.name}</TooltipContent> </Tooltip> </TooltipProvider> - ); + ), [row.original.name]); }Also applies to: 35-49, 56-67
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileSection.tsx (1)
82-84: Enhance grid accessibility and responsiveness.The grid layout should include ARIA roles and better responsive design for different screen sizes.
- <div className="grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-4 gap-4"> + <div + role="grid" + className="grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-4 gap-4" + aria-label="Uploaded files grid" + >apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx (1)
30-50: Optimize loading state transitions.The loading state could be more granular to prevent unnecessary full-page skeleton loading.
+ const [isInitialLoad, setIsInitialLoad] = useState(true); + + useEffect(() => { + if (!isLoading && isInitialLoad) { + setIsInitialLoad(false); + } + }, [isLoading]); + - if (isLoading) { + if (isLoading && isInitialLoad) {apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useUrlManagement.ts (2)
24-31: Consider enhancing URL validation.The current URL validation might allow some edge cases. Consider adding additional checks for:
- Maximum URL length
- Allowed protocols (e.g., only http/https)
- Valid characters in the URL
function isValidUrl(url: string): boolean { + const MAX_URL_LENGTH = 2048; + const ALLOWED_PROTOCOLS = ['http:', 'https:']; + + if (url.length > MAX_URL_LENGTH) return false; + try { - new URL(url); - return true; + const parsedUrl = new URL(url); + return ALLOWED_PROTOCOLS.includes(parsedUrl.protocol); } catch { return false; } }
97-103: Improve error handling for server errors.The error handling could be more specific about the type of error that occurred.
- if (!result) { - throw new Error("Failed to update URLs"); - } - - if (result.serverError) { - throw new Error(result.serverError || "Failed to update URLs"); - } + if (!result || result.serverError) { + const errorMessage = result?.serverError || "Failed to update URLs"; + throw new Error(`Server error: ${errorMessage}`); + }apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileCard.tsx (1)
50-51: Enhance file type detection.Consider using a more comprehensive approach for file type detection:
- Move file type detection to a separate utility function
- Add support for more image formats (e.g., svg, avif)
- Add support for more document types (e.g., doc, docx)
+const FILE_TYPES = { + IMAGE: /\.(jpg|jpeg|png|gif|webp|svg|avif)$/i, + PDF: /\.pdf$/i, + DOCUMENT: /\.(doc|docx|txt|rtf)$/i, +}; + +function getFileType(fileName: string) { + if (FILE_TYPES.IMAGE.test(fileName)) return 'image'; + if (FILE_TYPES.PDF.test(fileName)) return 'pdf'; + if (FILE_TYPES.DOCUMENT.test(fileName)) return 'document'; + return 'other'; +} + - const isImage = /\.(jpg|jpeg|png|gif|webp)$/i.test(fileName); - const isPdf = /\.pdf$/i.test(fileName); + const fileType = getFileType(fileName); + const isImage = fileType === 'image'; + const isPdf = fileType === 'pdf';packages/db/prisma/seed.ts (1)
372-399: Add error handling to seedEvidence function.While the function correctly seeds evidence data, it lacks error handling that's present in other seed functions. Consider wrapping the upsert operation in a try-catch block.
Apply this diff to add error handling:
async function seedEvidence() { const evidenceRequirements = await prisma.controlRequirement.findMany({ where: { type: RequirementType.evidence, }, }); console.log(`🔄 Processing ${evidenceRequirements.length} evidences`); for (const evidence of evidenceRequirements) { console.log(` ⏳ Processing evidence: ${evidence.name}...`); + try { await prisma.evidence.upsert({ where: { id: evidence.id, }, update: { name: evidence.name, description: evidence.description, }, create: { id: evidence.id, name: evidence.name, description: evidence.description, }, }); + console.log(` ✅ Evidence ${evidence.name} processed`); + } catch (error) { + console.error(` ❌ Error processing evidence ${evidence.name}:`, error); + if (error instanceof Error) { + console.error(` Error details: ${error.message}`); + } + } } }packages/db/prisma/migrations/20250220181557_name_required/migration.sql (1)
1-10: LGTM! Consider wrapping in a transaction for safety.The migration safely handles the transition to a NOT NULL column by first updating NULL values to empty strings. This prevents any potential data loss or migration failures.
Consider wrapping the migration in a transaction to ensure atomicity:
+BEGIN; UPDATE "ControlRequirement" SET "name" = '' WHERE "name" IS NULL; ALTER TABLE "ControlRequirement" ALTER COLUMN "name" SET NOT NULL, ALTER COLUMN "name" SET DEFAULT ''; +COMMIT;packages/db/prisma/schema.prisma (1)
1028-1049: New OrganizationEvidence Model.
TheOrganizationEvidencemodel captures organization-specific evidence with fields forfileUrlsandadditionalUrls, along with links to theOrganizationandEvidencemodels.
The separation betweenfileUrlsandadditionalUrlssuggests they serve different purposes—ensure this distinction is well documented to avoid confusion later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/app/languine.lockis excluded by!**/*.lockbun.lockbis excluded by!**/bun.lockbyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (61)
apps/app/package.json(1 hunks)apps/app/src/actions/framework/select-frameworks-action.ts(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/(home)/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/SingleControl.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/data-table/columns.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/data-table/data-table-header.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/data-table/data-table.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/page.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getAllOrganizationControlRequirements.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getEvidence.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidence.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidenceTasks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/SkeletonTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table-header.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/types.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/deleteEvidenceFile.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/getEvidenceFileUrl.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/updateEvidenceUrls.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/uploadEvidenceFile.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileCard.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileIcon.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileUpload.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileDelete.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFilePreview.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileUpload.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useOrganizationEvidence.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useUrlManagement.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/types.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/hooks/useEvidenceTasks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/integrations/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/(overview)/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/(overview)/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/[riskId]/layout.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/settings/layout.tsx(1 hunks)apps/app/src/components/main-menu.tsx(7 hunks)apps/app/src/locales/en.ts(1 hunks)apps/app/src/locales/es.ts(1 hunks)apps/app/src/locales/fr.ts(1 hunks)apps/app/src/locales/no.ts(1 hunks)apps/app/src/locales/pt.ts(1 hunks)apps/app/src/types/actions.ts(1 hunks)package.json(2 hunks)packages/data/controls/soc2.json(19 hunks)packages/db/prisma/migrations/20250220181245_add_evidence_table/migration.sql(1 hunks)packages/db/prisma/migrations/20250220181557_name_required/migration.sql(1 hunks)packages/db/prisma/migrations/20250220182223_fix_evidence_tables/migration.sql(1 hunks)packages/db/prisma/schema.prisma(4 hunks)packages/db/prisma/seed.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/app/src/app/[locale]/(app)/(dashboard)/controls/page.tsx
✅ Files skipped from review due to trivial changes (11)
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/page.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/risk/[riskId]/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/people/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/(home)/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/integrations/page.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/settings/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/(overview)/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/risk/(overview)/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/SkeletonTable.tsx
🔇 Additional comments (44)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/uploadEvidenceFile.ts (1)
64-66: File name sanitization looks good
Replacing non-alphanumeric characters with underscores ensures safer file paths and prevents path injection vulnerabilities. This strategy is approved.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/types.ts (1)
1-7: LGTM! Well-structured type definition.The
EvidenceTaskRowtype correctly extendsOrganizationEvidenceand adds a properly typedevidenceproperty.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/page.tsx (1)
1-11: LGTM! Clean and concise page component.The component follows Next.js conventions and correctly handles the dynamic route parameter.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/layout.tsx (1)
1-20: LGTM! Well-structured layout component.The component follows Next.js conventions, properly handles translations, and maintains consistent layout structure.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/layout.tsx (1)
12-12: LGTM! Consistent layout centering.The addition of
m-autoclass maintains visual consistency with other layout components.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/hooks/useEvidenceTasks.ts (2)
10-24: LGTM! Well-structured error handling and data access.The function implements thorough error handling and safe data access patterns.
26-37: LGTM! Well-implemented SWR hook.The hook follows React conventions and implements proper caching strategy with SWR.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidenceTasks.ts (1)
24-29: LGTM! Good authorization check implementation.The authorization check properly validates the user's organization context before proceeding with the database query.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx (1)
1-126: LGTM! Well-structured component with good UX.The component is well-implemented with:
- Clear separation of concerns using custom hooks
- Proper accessibility attributes
- Good error handling and user feedback
- Responsive design with proper spacing and layout
apps/app/src/components/main-menu.tsx (1)
31-31: LGTM! Evidence menu item added correctly.The evidence menu item is properly integrated with:
- Icon mapping using
Icons.Evidence- Translation key using
t("sidebar.evidence")- Consistent formatting with other menu items
Also applies to: 177-181
apps/app/src/locales/en.ts (1)
736-740: LGTM! Evidence translations are clear and consistent.The translations are well-written and follow the same pattern as other sections in the file.
apps/app/src/locales/no.ts (1)
717-720: LGTM! Norwegian translations are consistent.The translations maintain consistency with the Norwegian language style used throughout the file.
apps/app/src/locales/pt.ts (1)
717-720: LGTM!The new
evidencesection is properly added with correct Portuguese translations for the title and description.apps/app/src/locales/es.ts (1)
717-720: LGTM!The new
evidencesection is properly added with correct Spanish translations for the title and description.apps/app/src/locales/fr.ts (1)
717-720: LGTM!The new
evidencesection is properly added with correct French translations for the title and description.package.json (2)
16-16: New Dev Dependency for Lodash Types.
The addition of"@types/lodash": "^4.17.15"in the devDependencies enhances TypeScript support for lodash. This helps catch type issues during development.
29-29: New AWS SDK S3 Client Dependency.
The inclusion of"@aws-sdk/client-s3": "^3.750.0"under dependencies introduces S3 integration functionality. Please ensure that this version is compatible with your other AWS dependencies.packages/db/prisma/migrations/20250220181245_add_evidence_table/migration.sql (2)
1-12: Evidence Table Creation.
The "Evidence" table is defined with standard fields such asid,name,description, and timestamp fields. Note that whilecreatedAthas a default value,updatedAtdoes not—if automatic initial assignment is desired, you might consider adding a default value or ensuring your application logic handles it.
14-38: OrganizationEvidence Table and Index Setup.
The "OrganizationEvidence" table is created with fields includingpublished(defaulted tofalse) and an array forattachments(which is later dropped in a subsequent migration). The index definitions and the foreign key constraint linkingorganizationIdto the "Organization" table are properly set up.packages/db/prisma/migrations/20250220182223_fix_evidence_tables/migration.sql (2)
11-15: Modifying Evidence and ControlRequirement Tables.
On line 11, adding theevidenceIdcolumn to "ControlRequirement" is straightforward. DroppingfileUrlsandlinksfrom "Evidence" (lines 14-15) aligns with the updated data model. Make sure that any code referring to these columns is updated accordingly.
23-27: Updating Foreign Key Constraints.
The new foreign key constraints on "ControlRequirement" and "OrganizationEvidence" (lines 23-27) properly enforce referential integrity with the "Evidence" table. Verify that these cascading rules (ON DELETE SET NULLandON DELETE CASCADE) are consistent with your application’s business logic.apps/app/package.json (1)
15-15: AWS S3 Request Presigner Addition.
The addition of"@aws-sdk/s3-request-presigner": "^3.750.0"expands the S3 integration capabilities by enabling presigned URL generation. Confirm that its usage in the codebase aligns well with the previously added S3 client dependency to ensure consistent behavior.packages/db/prisma/schema.prisma (2)
923-938: ControlRequirement Model Updates.
In theControlRequirementmodel, several improvements have been made:
- The
namefield now defaults to an empty string, ensuring a non-null value.- Timestamp fields
createdAtandupdatedAtare explicitly defined for record tracking.- The addition of the optional
evidenceIdfield along with its relation to theEvidencemodel integrates evidence management into control requirements.Ensure that any application logic expecting these values is updated accordingly.
1013-1027: New Evidence Model.
The newEvidencemodel is introduced with core fields such asname,description,createdAt, andupdatedAt, and it establishes relations to bothControlRequirementandOrganizationEvidencemodels.
One minor note: the unique index onidis redundant sinceidis already the primary key; however, this does not affect functionality.packages/data/controls/soc2.json (19)
799-800: Added descriptive name for "CC7.3-evidence": "Recovery Records".
This addition clarifies that the evidence pertains to recovery testing by explicitly labeling it.
825-826: Added explicit name for "CC7.4-evidence": "Incident Analysis Records".
The new name improves clarity by clearly indicating that the evidence relates to the analysis of incidents.
851-852: Defined evidence label for "CC7.5-evidence": "Incident Communication Records".
This update makes it clear that the evidence is focused on communications during security events.
877-878: Added descriptive name for "CC8.1-evidence": "Change Request Logs".
The enhanced naming provides an explicit title for evidence related to change management requests.
903-904: Enhanced evidence label for "CC9.1-evidence": "Business Continuity Plans".
This update improves readability by succinctly indicating that the evidence documents business continuity planning.
929-930: Updated evidence name for "CC9.2-evidence": "Vendor Risk Assessment Records".
This renaming adds clarity by specifying that the records document vendor risk assessments.
955-956: Added detailed name for "CC9.9-evidence": "Business Continuity and Disaster Recovery Testing Records".
The extended title conveys a comprehensive scope, ensuring that test results and related documentation are clearly identified.
981-982: Introduced descriptive evidence label for "A1.1-evidence": "Uptime Reports".
This clear naming aids in quickly identifying evidence concerning system availability.
1007-1008: Added clear name for "A1.2-evidence": "Capacity Reports".
The update clearly denotes that the evidence relates to reports on system capacity, enhancing documentation clarity.
1033-1034: Defined evidence label for "A1.3-evidence": "Incident Recovery Records".
This improvement provides better context for evidence documenting outcomes from incident recovery processes.
1059-1060: Added explicit name for "C1.1-evidence": "Data Classification Records".
The new label makes it easier to identify evidence related to the classification of confidential data.
1085-1086: Defined a clear evidence name for "C1.2-evidence": "Access Logs".
This change enhances understanding by specifying that the records pertain to access control activities.
1111-1112: Added a descriptive name for "C1.3-evidence": "Disposal Records".
This update improves clarity regarding evidence associated with secure data disposal procedures.
1137-1138: Introduced clear evidence name for "PI1.1-evidence": "Data Validation Records".
This addition assists in identifying documentation that concerns the validation and accuracy of processed data.
1163-1164: Enhanced evidence label for "PI1.2-evidence": "Data Processing Logs".
The updated name makes it straightforward to recognize records detailing data processing activities.
1189-1190: Added precise evidence name for "PI1.3-evidence": "Exception Logs".
This update reinforces the identification of evidence related to processing exceptions and error handling.
1215-1216: Defined evidence label for "P1.1-evidence": "Privacy Notice".
Adding this clear identifier supports consistency in privacy-related documentation across the framework.
1241-1242: Added descriptive name for "P1.2-evidence": "Consent Records".
This change improves clarity by specifying that the evidence relates to the records of individual consent.
1267-1268: Updated evidence label for "P1.3-evidence": "Retention Schedules".
The renaming enhances the clarity of documentation concerning data retention and disposal processes.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getEvidence.ts (1)
7-19: LGTM! Well-structured schema validation and metadata tracking.The schema validation and metadata tracking are appropriately defined for the evidence retrieval action.
| try { | ||
| const evidence = await db.organizationEvidence.findFirst({ | ||
| where: { | ||
| id: evidenceId, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add organization check to prevent unauthorized access.
The database query should include the user's organizationId in the where clause to ensure users can only access evidence from their own organization.
Apply this diff to fix the security issue:
const evidence = await db.organizationEvidence.findFirst({
where: {
id: evidenceId,
+ organizationId: user.organizationId,
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const evidence = await db.organizationEvidence.findFirst({ | |
| where: { | |
| id: evidenceId, | |
| }, | |
| }); | |
| try { | |
| const evidence = await db.organizationEvidence.findFirst({ | |
| where: { | |
| id: evidenceId, | |
| organizationId: user.organizationId, | |
| }, | |
| }); |
| evidenceId: z.string(), | ||
| urls: z.array(z.string().url()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider additional URL validation and sanitization.
While Zod's URL validation is good, consider additional checks for:
- Maximum URL length
- Allowed protocols (https only)
- URL sanitization
evidenceId: z.string(),
- urls: z.array(z.string().url()),
+ urls: z.array(
+ z.string()
+ .url()
+ .max(2048)
+ .refine(
+ (url) => url.startsWith("https://"),
+ "Only HTTPS URLs are allowed"
+ )
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| evidenceId: z.string(), | |
| urls: z.array(z.string().url()), | |
| evidenceId: z.string(), | |
| urls: z.array( | |
| z.string() | |
| .url() | |
| .max(2048) | |
| .refine( | |
| (url) => url.startsWith("https://"), | |
| "Only HTTPS URLs are allowed" | |
| ) | |
| ), |
| try { | ||
| // Get the presigned URL | ||
| const urlResult = (await getUploadUrl({ | ||
| evidenceId, | ||
| fileName: file.name, | ||
| fileType: file.type, | ||
| })) as ServerResponse; | ||
|
|
||
| // Validate server response | ||
| if (!urlResult?.data?.data?.uploadUrl) { | ||
| throw new Error(urlResult?.serverError || "Failed to get upload URL"); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Handle potential race conditions in URL generation.
The URL generation process should be protected against race conditions when multiple uploads are triggered in quick succession.
🏁 Script executed:
#!/bin/bash
# Check for potential race conditions in URL generation
ast-grep --pattern 'getUploadUrl($$$)'Length of output: 623
Critical: Address Race Conditions in getUploadUrl Call
The call to getUploadUrl on lines 50-61 does not implement any mechanism (e.g., locking, debouncing, or request serialization) to prevent race conditions if multiple file uploads are initiated simultaneously. Please add a safeguard to ensure that concurrent uploads do not trigger overlapping URL generation requests.
- Verify that each upload operation receives a uniquely generated URL.
- Consider implementing a mutual exclusion or debouncing strategy around
getUploadUrl.
| onUploadProgress: (progressEvent: AxiosProgressEvent): void => { | ||
| if (progressEvent.total) { | ||
| const percentCompleted = Math.round( | ||
| (progressEvent.loaded * 100) / progressEvent.total | ||
| ); | ||
| console.debug(`Upload progress: ${percentCompleted}%`); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose upload progress to the UI.
Progress tracking is currently only logged to console. Consider exposing it to the UI for better user experience.
+ const [uploadProgress, setUploadProgress] = useState<number>(0);
+
onUploadProgress: (progressEvent: AxiosProgressEvent): void => {
if (progressEvent.total) {
const percentCompleted = Math.round(
(progressEvent.loaded * 100) / progressEvent.total
);
- console.debug(`Upload progress: ${percentCompleted}%`);
+ setUploadProgress(percentCompleted);
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onUploadProgress: (progressEvent: AxiosProgressEvent): void => { | |
| if (progressEvent.total) { | |
| const percentCompleted = Math.round( | |
| (progressEvent.loaded * 100) / progressEvent.total | |
| ); | |
| console.debug(`Upload progress: ${percentCompleted}%`); | |
| } | |
| }, | |
| const [uploadProgress, setUploadProgress] = useState<number>(0); | |
| onUploadProgress: (progressEvent: AxiosProgressEvent): void => { | |
| if (progressEvent.total) { | |
| const percentCompleted = Math.round( | |
| (progressEvent.loaded * 100) / progressEvent.total | |
| ); | |
| setUploadProgress(percentCompleted); | |
| } | |
| }, |
| async (file: File): Promise<void> => { | ||
| if (!file || !evidenceId) { | ||
| toast({ | ||
| title: "Error", | ||
| description: "Missing file or evidence ID", | ||
| variant: "destructive", | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file type and size validation.
The function should validate file types and sizes before initiating the upload to prevent unnecessary server requests.
async (file: File): Promise<void> => {
+ const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100MB
+ const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'application/pdf'];
+
+ if (file.size > MAX_FILE_SIZE) {
+ toast({
+ title: "Error",
+ description: "File size exceeds 100MB limit",
+ variant: "destructive",
+ });
+ return;
+ }
+
+ if (!ALLOWED_TYPES.includes(file.type)) {
+ toast({
+ title: "Error",
+ description: "Invalid file type. Allowed: JPG, PNG, PDF",
+ variant: "destructive",
+ });
+ return;
+ }
+
if (!file || !evidenceId) {Committable suggestion skipped: line range outside the PR's diff.
| /* | ||
| Warnings: | ||
|
|
||
| - You are about to drop the column `fileUrls` on the `Evidence` table. All the data in the column will be lost. | ||
| - You are about to drop the column `links` on the `Evidence` table. All the data in the column will be lost. | ||
| - You are about to drop the column `attachments` on the `OrganizationEvidence` table. All the data in the column will be lost. | ||
| - Added the required column `evidenceId` to the `OrganizationEvidence` table without a default value. This is not possible if the table is not empty. | ||
|
|
||
| */ |
There was a problem hiding this comment.
Data Loss Warning and Non-Nullable Column Concerns.
The warning block clearly indicates that dropping columns (fileUrls, links, and attachments) will result in data loss. Also, note the addition of a non-nullable evidenceId column in "OrganizationEvidence" (line 7) without a default value.
Ensure you have a migration strategy (such as a data backfill or a brief downtime) to handle existing data before running this migration.
| -- AlterTable | ||
| ALTER TABLE "OrganizationEvidence" DROP COLUMN "attachments", | ||
| ADD COLUMN "additionalUrls" TEXT[], | ||
| ADD COLUMN "evidenceId" TEXT NOT NULL, | ||
| ADD COLUMN "fileUrls" TEXT[]; |
There was a problem hiding this comment.
Adjustments to OrganizationEvidence.
The script drops the attachments column and adds three new columns: additionalUrls (for supplementary URLs), a mandatory evidenceId, and fileUrls. The new evidenceId being NOT NULL could cause issues if the table already contains data.
Consider performing a data migration or supplying a default value to avoid migration failures on non-empty tables.
| .action(async ({ ctx, parsedInput }) => { | ||
| const { user } = ctx; | ||
| const { evidenceId } = parsedInput; | ||
|
|
||
| if (!user.organizationId) { | ||
| return { | ||
| success: false, | ||
| error: "Not authorized - no organization found", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add organization ownership check for evidence.
While checking for user's organization is good, the code should also verify that the evidence belongs to the user's organization to prevent unauthorized access.
Apply this diff to add the organization ownership check:
if (!user.organizationId) {
return {
success: false,
error: "Not authorized - no organization found",
};
}
+
+ // Verify evidence belongs to user's organization
+ const evidence = await db.organizationEvidence.findFirst({
+ where: {
+ id: evidenceId,
+ organizationId: user.organizationId,
+ },
+ });
+
+ if (!evidence) {
+ return {
+ success: false,
+ error: "Evidence not found or not authorized",
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .action(async ({ ctx, parsedInput }) => { | |
| const { user } = ctx; | |
| const { evidenceId } = parsedInput; | |
| if (!user.organizationId) { | |
| return { | |
| success: false, | |
| error: "Not authorized - no organization found", | |
| }; | |
| } | |
| .action(async ({ ctx, parsedInput }) => { | |
| const { user } = ctx; | |
| const { evidenceId } = parsedInput; | |
| if (!user.organizationId) { | |
| return { | |
| success: false, | |
| error: "Not authorized - no organization found", | |
| }; | |
| } | |
| // Verify evidence belongs to user's organization | |
| const evidence = await db.organizationEvidence.findFirst({ | |
| where: { | |
| id: evidenceId, | |
| organizationId: user.organizationId, | |
| }, | |
| }); | |
| if (!evidence) { | |
| return { | |
| success: false, | |
| error: "Evidence not found or not authorized", | |
| }; | |
| } |
| try { | ||
| const evidence = await db.organizationEvidence.findFirst({ | ||
| where: { | ||
| id: evidenceId, | ||
| }, | ||
| }); | ||
|
|
||
| if (!evidence) { | ||
| return { | ||
| success: false, | ||
| error: "Evidence not found", | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| data: evidence, | ||
| }; | ||
| } catch (error) { | ||
| console.error("Error fetching evidence:", error); | ||
| return { | ||
| success: false, | ||
| error: "Failed to fetch evidence", | ||
| }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant database query.
Since we've added the organization ownership check earlier, this database query becomes redundant and can be simplified.
Apply this diff to simplify the code:
- try {
- const evidence = await db.organizationEvidence.findFirst({
- where: {
- id: evidenceId,
- },
- });
-
- if (!evidence) {
- return {
- success: false,
- error: "Evidence not found",
- };
- }
-
return {
success: true,
data: evidence,
};
- } catch (error) {
- console.error("Error fetching evidence:", error);
- return {
- success: false,
- error: "Failed to fetch evidence",
- };
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const evidence = await db.organizationEvidence.findFirst({ | |
| where: { | |
| id: evidenceId, | |
| }, | |
| }); | |
| if (!evidence) { | |
| return { | |
| success: false, | |
| error: "Evidence not found", | |
| }; | |
| } | |
| return { | |
| success: true, | |
| data: evidence, | |
| }; | |
| } catch (error) { | |
| console.error("Error fetching evidence:", error); | |
| return { | |
| success: false, | |
| error: "Failed to fetch evidence", | |
| }; | |
| } | |
| }); | |
| return { | |
| success: true, | |
| data: evidence, | |
| }; | |
| }); |
| const handleDelete = useCallback( | ||
| async (fileUrl: string) => { | ||
| try { | ||
| const response = await deleteEvidenceFile({ | ||
| evidenceId, | ||
| fileUrl, | ||
| }); | ||
|
|
||
| if (!response?.data) { | ||
| throw new Error("Failed to delete file"); | ||
| } | ||
|
|
||
| if (!response.data.success) { | ||
| throw new Error(response.data.error || "Failed to delete file"); | ||
| } | ||
|
|
||
| await onSuccess(); | ||
| toast({ | ||
| title: "Success", | ||
| description: "File deleted successfully", | ||
| }); | ||
| } catch (error) { | ||
| console.error("Error deleting file:", error); | ||
| toast({ | ||
| title: "Error", | ||
| description: | ||
| error instanceof Error ? error.message : "Failed to delete file", | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| }, | ||
| [evidenceId, onSuccess, toast] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make onSuccess call conditional.
Since we're making onSuccess optional, we should call it conditionally.
Apply this diff:
}
- await onSuccess();
+ if (onSuccess) {
+ await onSuccess();
+ }
toast({
title: "Success",
description: "File deleted successfully",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDelete = useCallback( | |
| async (fileUrl: string) => { | |
| try { | |
| const response = await deleteEvidenceFile({ | |
| evidenceId, | |
| fileUrl, | |
| }); | |
| if (!response?.data) { | |
| throw new Error("Failed to delete file"); | |
| } | |
| if (!response.data.success) { | |
| throw new Error(response.data.error || "Failed to delete file"); | |
| } | |
| await onSuccess(); | |
| toast({ | |
| title: "Success", | |
| description: "File deleted successfully", | |
| }); | |
| } catch (error) { | |
| console.error("Error deleting file:", error); | |
| toast({ | |
| title: "Error", | |
| description: | |
| error instanceof Error ? error.message : "Failed to delete file", | |
| variant: "destructive", | |
| }); | |
| } | |
| }, | |
| [evidenceId, onSuccess, toast] | |
| ); | |
| const handleDelete = useCallback( | |
| async (fileUrl: string) => { | |
| try { | |
| const response = await deleteEvidenceFile({ | |
| evidenceId, | |
| fileUrl, | |
| }); | |
| if (!response?.data) { | |
| throw new Error("Failed to delete file"); | |
| } | |
| if (!response.data.success) { | |
| throw new Error(response.data.error || "Failed to delete file"); | |
| } | |
| if (onSuccess) { | |
| await onSuccess(); | |
| } | |
| toast({ | |
| title: "Success", | |
| description: "File deleted successfully", | |
| }); | |
| } catch (error) { | |
| console.error("Error deleting file:", error); | |
| toast({ | |
| title: "Error", | |
| description: | |
| error instanceof Error ? error.message : "Failed to delete file", | |
| variant: "destructive", | |
| }); | |
| } | |
| }, | |
| [evidenceId, onSuccess, toast] | |
| ); |
Summary by CodeRabbit
New Features
Style